Add ToggleLineComment and ToggleMultilineComment service#37029
Add ToggleLineComment and ToggleMultilineComment service#37029armanio123 merged 22 commits intomicrosoft:masterfrom
Conversation
|
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
sandersn
added
the
For Milestone Bug
amcasey
left a comment
amcasey
left a comment
There was a problem hiding this comment.
Updating my status so I don't get nag mail about not reviewing this. 😉
|
@armanio123 @amcasey is this supposed to ship in TS 3.9 or 4.0? Is it appropriate to ship it in the RC but not the beta? There's no associated bug so I don't have a way to tell. |
|
This should target 4.0 at this point. |
|
What does this need to go into 4.0 now? |
armanio123
force-pushed
the
AddToggleCommentFeature
branch
from
e2e4575 to
413a3d3
Compare
|
Found a bug with uncomment that got fixed on the latest commit. This PR is ready to be merged as soon as I get an approval. |
This reverts commit 40751ba.
|
Thanks @uniqueiniquity for the review. I have addressed all of the changes. Let me know if any additional comments. |
amcasey
left a comment
amcasey
left a comment
There was a problem hiding this comment.
I'm pretty sure there's going to be a bug tail, but I think it's better to get this out and hear from actual customers than to rathole on corner cases.
|
|
||
| verify.toggleMultilineComment( | ||
| `let var1/* = 1; | ||
| let var2 *//*= 2; |
There was a problem hiding this comment.
I would be incredibly confused if this happened to me. Without having tried it, my guess would be that I'd expect the existing span to be uncommented.
There was a problem hiding this comment.
On a related note, why not combine contiguous comment ranges?
There was a problem hiding this comment.
Yeah, this is one of those scenarios that are difficult to decide the best approach.
IMO, the only reason to not merge the comments is because when we want to reverse the operation we might get a different result:. For example when using jsdoc:
[|const foo = 1;
/** some documentation
* more doc.
*/
function bar() {}|]this will result on:
/*const foo = 1;
*//** some documentation
* more doc.
*//*
function bar() {}*/Looks messy but in a way it respects existing comments. If you want to uncomment either foo or bar it should be ok. If we merge comments, the jsdoc syntax will be all mess up.
This is not only for jsdoc, pretty much any existing comment will experience this issue when uncommenting.
7 participants
Implements toggleLineComment and toggleMultilineComment services. Considers jsx as well.